Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[wasm] Download Symbols from microsoft symbol server #40690

Merged
merged 24 commits into from
Sep 15, 2020
Merged

[wasm] Download Symbols from microsoft symbol server #40690

merged 24 commits into from
Sep 15, 2020

Conversation

thaystg
Copy link
Member

@thaystg thaystg commented Aug 12, 2020

Creating a draft to download symbols from microsoft symbol server when there is an exception and exceptions are turned on on debugger.

This is a workaround while VS doesn't work on it, which should be the final solution.

If you think that is in the right way, there is still a to do list:

  • Read url from config file, or receive as an startup parameter.
  • Create a flag to enable and disable this functionality, because this may cause some slowness when creating the callstack of the exception.

The callstack exception without this PR:
image

The callstack exception with this PR:
image

@thaystg
Creating a draft to download symbols from microsoft symbol server whe…
67075a6 
…n there is an exception and exceptions are turned on on debugger.

This is a workaround while VS doesn't work on it, which should be the final solution.
@thaystg thaystg requested review from radical and lewing August 12, 2020 01:03
@thaystg thaystg mentioned this pull request Aug 12, 2020
Merged
lewing
lewing previously requested changes Aug 13, 2020
View reviewed changes
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
@thaystg thaystg requested a review from lewing August 14, 2020 18:32
@thaystg
Copy link
Member Author

thaystg commented Aug 14, 2020

I'm not sure what is the best way to receive the symbol server url, I was thinking to receive in the same way that we receive browserUri, and pass to DevToolsProxy in run method. If anyone has any better idea, please let me know.

radical
radical approved these changes Aug 15, 2020
View reviewed changes
Copy link
Member

@radical radical left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only nitpicks, nothing to block merging. 👍

src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs Outdated Show resolved Hide resolved
@radical
Copy link
Member

radical commented Aug 15, 2020

I'm not sure what is the best way to receive the symbol server url, I was thinking to receive in the same way that we receive browserUri, and pass to DevToolsProxy in run method. If anyone has any better idea, please let me know.

What does the server url depend on? Does it change?

@thaystg
Copy link
Member Author

thaystg commented Aug 17, 2020

I'm not sure what is the best way to receive the symbol server url, I was thinking to receive in the same way that we receive browserUri, and pass to DevToolsProxy in run method. If anyone has any better idea, please let me know.

What does the server url depend on? Does it change?

On VsWindows for example, the user can choose a list of symbol server URL. From Microsoft I think that this is the only one.

image

@thaystg thaystg marked this pull request as ready for review August 17, 2020 14:09
@radical
Copy link
Member

radical commented Aug 24, 2020

re:url, you could send a custom message to the proxy to pass the url, and handle that in MonoProxy, similar to https://github.com/dotnet/runtime/blob/master/src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs#L340-L341 . We send that one in tests - https://github.com/dotnet/runtime/blob/master/src/mono/wasm/debugger/DebuggerTestSuite/Support.cs#L878 .

How will you get this url from VS though, to pass on to the proxy?

@thaystg
Copy link
Member Author

thaystg commented Aug 24, 2020

The custom message that you suggested is the same that I think we should do.
I have no idea how to get this url from VS, but if we create the custom message, VS team can call this message passing to Proxy the urls.

@thaystg @lewing
Update src/mono/wasm/debugger/BrowserDebugProxy/MonoProxy.cs
429907f 
Co-authored-by: Larry Ewing <lewing@microsoft.com>
@thaystg
Copy link
Member Author

thaystg commented Aug 26, 2020

@lewing , I'm discussing with Diego about how they will pass this info for DebuProxy.

@thaystg
Adding support for receive the urlSymbolServer as a parameter in the …
7598ac9 
…request as discussed with Diego:

Diego: The response from vs code is that this shouldn't live in js-debug, so I think that passing it around in inspectUri would be the best approach for us
@thaystg
Merge remote-tracking branch 'upstream/master' into thays_load_symbol…
12f45c9 
…s_from_ms_symbol_server
@thaystg
Copy link
Member Author

thaystg commented Sep 4, 2020

@lewing @radical now this is ready to be merged.
Diego asked to pass the urlSymbolServer as a parameter, like this they don't need to change js-debug. In any case, I let the protocol extension if we need this later and accept the parameter too.

@radical
Copy link
Member

radical commented Sep 14, 2020

Is there anything blocking this now?

@thaystg
Copy link
Member Author

thaystg commented Sep 14, 2020

I don't think so, I just removed the default microsoft symbol server URL, then we can try to merge it on rc 2.
@lewing can we merge now?

@thaystg
Adding workaround on dotnet/runtime while PR 686 is not merged on Cec…
729887b 
…il. When it's merged we will not get any side effect with this workaroud.
@thaystg
Copy link
Member Author

thaystg commented Sep 15, 2020

@lewing I removed the default URL of MS Symbol Server, so now, this will not affect the user unless he changes the LaunchSettings.json to use this InspectUri: "inspectUri": "{wsProtocol}://{url.hostname}:{url.port}/_framework/debug/ws-proxy?browser={browserInspectUri}?urlSymbolServer=http://msdl.microsoft.com/download/symbols"
Then the new implementation will be effective.

@thaystg thaystg merged commit eb6bcde into dotnet:master Sep 15, 2020
@thaystg
Copy link
Member Author

thaystg commented Sep 15, 2020

/backport to release/5.0-rc2

@github-actions
Copy link
Contributor

Started backporting to release/5.0-rc2: https://github.com/dotnet/runtime/actions/runs/256085151

@github-actions github-actions bot mentioned this pull request Sep 15, 2020
Closed
@thaystg thaystg mentioned this pull request Sep 30, 2020
Merged
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lewing lewing lewing left review comments

@radical radical radical approved these changes

@marek-safar marek-safar Awaiting requested review from marek-safar

Assignees
No one assigned
Labels
area-Debugger-mono
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thaystg @radical @lewing @jkoritzinsky @Dotnet-GitSync-Bot